Introduce assorted Reactor `StepVerifier` Refaster rules #1132

werli · 2024-04-11T20:56:33Z

Summary

This PR introduces a bunch of rules associated to StepVerifier.Asserations and a trivial StepVerifier rule.

Example

I spotted the following interesting pattern:

Mono.empty()
    .as(StepVerifier::create)
    .expectError(SpecificException.class)
    .verifyThenAssertThat()
    .hasOperatorErrorWithMessage("msg");

Which made me find the interesting StepVerifier#Assertions API. It's quite low-level, and IMO not as straightforward to read and write.

The example itself is most often represented in our code-base using AssertJ's richer API:

Mono.empty()
    .as(StepVerifier::create)
    .verifyErrorSatisfies(t ->
        assertThat(t)
        .isInstanceOf(SpecificException.class)
        .hasMessage("msg"));

Suggested commit message:

Introduce assorted Reactor `StepVerifier` Refaster rules (#1132)

github-actions · 2024-04-11T20:58:27Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

werli · 2024-04-11T21:00:32Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /**
+   * Prefer {@link StepVerifier.LastStep#verifyErrorSatisfies(Consumer)} with AssertJ over more
+   * contrived alternatives.
+   */
+  static final class StepVerifierLastStepVerifyErrorSatisfiesAssertJ {
+    @BeforeTemplate
+    void before(StepVerifier.LastStep step, Class<? extends Throwable> clazz, String message) {
+      Refaster.anyOf(
+          step.expectError()
+              .verifyThenAssertThat()
+              .hasOperatorErrorOfType(clazz)
+              .hasOperatorErrorWithMessage(message),
+          step.expectError(clazz).verifyThenAssertThat().hasOperatorErrorWithMessage(message),
+          step.expectErrorMessage(message).verifyThenAssertThat().hasOperatorErrorOfType(clazz));
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    void after(StepVerifier.LastStep step, Class<? extends Throwable> clazz, String message) {
+      step.verifyErrorSatisfies(
+          t -> Assertions.assertThat(t).isInstanceOf(clazz).hasMessage(message));
+    }
+  }


Asserting both class and message blocks one to use the recommended StepVerifier.Step#verifyError(Class) or StepVerifier.Step#verifyErrorMessage(String). Choosing an alternative isn't easy since there are many possibilities. Out of all the options, AssertJ's assertions have the richest API, so I'd recommend this.

The StepVerifierVerify rule introduced in this PR aims to rewrite the use-cases in which only one of class or message are asserted by dropping the StepVerifier.Assertions type which allows other rules to rewrite further.

werli · 2024-04-11T21:08:26Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /**
+   * Prefer {@link StepVerifier.LastStep#verifyErrorMatches(Predicate)} over more verbose
+   * alternatives.
+   */
+  static final class StepVerifierLastStepVerifyErrorMatchesAssertions {
+    @BeforeTemplate
+    void before(StepVerifier.LastStep step, Predicate<Throwable> predicate) {
+      step.expectError().verifyThenAssertThat().hasOperatorErrorMatching(predicate);
+    }
+
+    @AfterTemplate
+    void after(StepVerifier.LastStep step, Predicate<Throwable> predicate) {
+      step.verifyErrorMatches(predicate);
+    }
+  }


This case is not exhaustive due to the overload of StepVerifier.LastStep#expectedError, but IMO covers sufficiently.

We can collapse this one into the preceding method.

werli · 2024-04-11T21:09:28Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /** Don't unnecessarily invoke {@link StepVerifier#verifyLater()} multiple times. */
+  static final class StepVerifierVerifyLater<T> {
+    @BeforeTemplate
+    StepVerifier before(StepVerifier stepVerifier) {
+      return stepVerifier.verifyLater().verifyLater();
+    }
+
+    @AfterTemplate
+    StepVerifier after(StepVerifier stepVerifier) {
+      return stepVerifier.verifyLater();
+    }
+  }


The API spec mentions this is a noop. I didn't fully understand why this API may be useful for in the first place.

github-actions · 2024-04-11T21:15:23Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-04-12T07:11:43Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added a commit. Tnx @werli!

Stephan202 · 2024-04-12T16:03:30Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /**
+   * Prefer {@link StepVerifier.LastStep#verifyErrorMatches(Predicate)} over more verbose
+   * alternatives.
+   */
+  static final class StepVerifierLastStepVerifyErrorMatchesAssertions {
+    @BeforeTemplate
+    void before(StepVerifier.LastStep step, Predicate<Throwable> predicate) {
+      step.expectError().verifyThenAssertThat().hasOperatorErrorMatching(predicate);
+    }
+
+    @AfterTemplate
+    void after(StepVerifier.LastStep step, Predicate<Throwable> predicate) {
+      step.verifyErrorMatches(predicate);
+    }
+  }


We can collapse this one into the preceding method.

Stephan202 · 2024-04-12T16:18:11Z

...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java

+    Mono.empty()
+        .as(StepVerifier::create)
+        .expectError()
+        .verifyThenAssertThat()
+        .hasOperatorErrorOfType(IllegalStateException.class)
+        .hasOperatorErrorWithMessage("foo");
+    Mono.empty()
+        .as(StepVerifier::create)
+        .expectError(IllegalStateException.class)
+        .verifyThenAssertThat()
+        .hasOperatorErrorWithMessage("bar");
+    Mono.empty()
+        .as(StepVerifier::create)
+        .expectErrorMessage("baz")
+        .verifyThenAssertThat()
+        .hasOperatorErrorOfType(IllegalStateException.class);


We can use three different Throwable types like a bit further up.

Stephan202 · 2024-04-12T16:22:47Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+   */
+  static final class StepVerifierVerify {
+    @BeforeTemplate
+    void before(StepVerifier stepVerifier) {


While in this case arguably incorrect, we do generally match any expression, even if that can lead to compilation errors. (At least this will force the user to clean up the code.)

I'll apply those changes to this PR, though I could also see us change our mind on that in the future.

My intent was indeed to avoid breaking code changes by only matching the "dangling" case in which the created StepVerifier.Assertions isn't used or cases we should clearly rewrite.

StepVerifier#verifyThenAssertThat has some valid yet niche usages, see internal usages. Sure, we could suppress the warning here, but IMO the warning would be a false positive. WDYT?

Okay - I've given this another thought. Let's proceed as-is, and violations could be suppressed through the warning. The internal use cases are quite sparse.

Discussed offline: I do think @werli has a good point. Ideally we have our cake and eat it too (i.e., have separate "aggressive" and "safe" modes). I'll add a comment describing the current thinking of what that could look like.

github-actions · 2024-04-12T17:33:55Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Nice improvements!

github-actions · 2024-05-02T06:09:52Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2024-05-02T06:09:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

werli · 2024-11-01T12:05:12Z

^ Rebased to resolve conflicts. IMO, we can proceed. 👍

github-actions · 2024-11-01T12:07:12Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-11-01T12:34:25Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added a commit, summarizing what we discussed offline. Let's merge after #1387 is merged and released.

github-actions · 2024-11-03T10:32:38Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2024-11-03T10:58:04Z

The build fails due to what appears to be a bug in JDK 23. Will require further investigation later ~today.

Stephan202 · 2024-11-04T07:23:07Z

The build fails due to what appears to be a bug in JDK 23. Will require further investigation later ~today.

Will require attaching a debugger, I think, for which I'll need to find more time. Observations so far:

The errors happen for {@link StepVerifier.LastStep#verify()} and {@link StepVerifier.LastStep#verify(Duration)}.
There are no errors for {@link StepVerifier.LastStep#verifyError()} and {@link StepVerifier.LastStep#verifyError(Class)}. (This is surprising: same (nested) class, also a pair of overloads.)
As expected, the issue doesn't trigger with -Xdoclint:-reference, as the relevant validation is then skipped altogether.
Based on a quick test with 22.0.1-graalce (which I happened to still have installed locally) the issue isn't present in JDK 22.

werli

The last changes LGTM, thanks @Stephan202 💪
Interesting bug investigation. What do we do in this scenario for error-prone-support? Change our code & report, or report & leave the PR open until it's fixed? The latter may take a while.

Stephan202 · 2024-11-04T19:17:50Z

Interesting bug investigation. What do we do in this scenario for error-prone-support? Change our code & report, or report & leave the PR open until it's fixed? The latter may take a while.

Discussed offline: certainly we'll want to proceed with this PR. Whatever workaround we go for, ideally we do link it to a bug report. This would likely entail the following steps:

Use a debugger to better understand and be able to describe the issue and, if possible, create a minimal reproducible example.
Create an account to file a JDK bug ticket.
File a bug ticket.

Depending on availability one of us will have a crack at this.

…rule

… rule

Nice

Stephan202 · 2024-11-11T12:41:22Z

Alright, I rebased and added a commit. Turns out the bug is in all versions preceding JDK 23: There are no StepVerifier.LastStep#verify overloads: #verify is defined on StepVerifier instead. 😅. Will merge once built.

github-actions · 2024-11-11T12:43:05Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2024-11-11T12:43:26Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

werli · 2024-11-11T13:10:06Z

💡💡💡 Thanks @Stephan202 💪

werli added the new feature label Apr 11, 2024

werli commented Apr 11, 2024

View reviewed changes

werli force-pushed the werli/reactor-verify-error-satisfies branch from 7395f93 to 0eea3ac Compare April 11, 2024 21:13

Stephan202 added this to the 0.17.0 milestone Apr 12, 2024

Stephan202 approved these changes Apr 12, 2024

View reviewed changes

rickie approved these changes May 2, 2024

View reviewed changes

rickie force-pushed the werli/reactor-verify-error-satisfies branch from 3742218 to fcbda80 Compare May 2, 2024 06:07

Stephan202 modified the milestones: 0.17.0, 0.18.0 Jul 20, 2024

Stephan202 modified the milestones: 0.18.0, 0.19.0 Aug 11, 2024

rickie modified the milestones: 0.19.0, 0.20.0 Oct 25, 2024

Venorcis approved these changes Nov 1, 2024

View reviewed changes

werli force-pushed the werli/reactor-verify-error-satisfies branch from fcbda80 to 8f803e0 Compare November 1, 2024 12:04

werli requested a review from Stephan202 November 1, 2024 13:11

Stephan202 approved these changes Nov 3, 2024

View reviewed changes

werli commented Nov 4, 2024

View reviewed changes

werli and others added 9 commits November 11, 2024 13:06

Introduce StepVerifierVerifyLater Refaster rule

1af4645

Introduce StepVerifierVerify{,Duration} Refaster rule

c93356e

Introduce StepVerifierLastStepVerifyErrorSatisfiesAssertJ Refaster …

3fbd08b

…rule

Introduce StepVerifierLastStepVerifyErrorMatchesAssertions Refaster…

bb6f51a

… rule

Run ./apply-error-prone-suggestions.sh

62d4f7e

Nice

Suggestions

55528aa

Indicate that a subset of these rules have breaking behavior

f82c671

Write down current thinking

f2830a5

Doh!

cee4b2c

Stephan202 force-pushed the werli/reactor-verify-error-satisfies branch from f7b1121 to cee4b2c Compare November 11, 2024 12:41

Stephan202 merged commit da9b313 into master Nov 11, 2024
16 checks passed

Stephan202 deleted the werli/reactor-verify-error-satisfies branch November 11, 2024 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce assorted Reactor `StepVerifier` Refaster rules #1132

Introduce assorted Reactor `StepVerifier` Refaster rules #1132

werli commented Apr 11, 2024 •

edited

Loading

github-actions bot commented Apr 11, 2024

werli Apr 11, 2024

werli Apr 11, 2024

Stephan202 Apr 12, 2024

werli Apr 11, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 12, 2024

Stephan202 left a comment

Stephan202 Apr 12, 2024

Stephan202 Apr 12, 2024

Stephan202 Apr 12, 2024

werli Apr 15, 2024

werli Nov 1, 2024

Stephan202 Nov 3, 2024

github-actions bot commented Apr 12, 2024

rickie left a comment •

edited

Loading

github-actions bot commented May 2, 2024

sonarqubecloud bot commented May 2, 2024

werli commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

Stephan202 left a comment

github-actions bot commented Nov 3, 2024

Stephan202 commented Nov 3, 2024

Stephan202 commented Nov 4, 2024

werli left a comment •

edited

Loading

Stephan202 commented Nov 4, 2024

Stephan202 commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

sonarqubecloud bot commented Nov 11, 2024

werli commented Nov 11, 2024

Introduce assorted Reactor StepVerifier Refaster rules #1132

Introduce assorted Reactor StepVerifier Refaster rules #1132

Conversation

werli commented Apr 11, 2024 • edited Loading

Summary

Example

Suggested commit message:

github-actions bot commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 12, 2024

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 12, 2024

rickie left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented May 2, 2024

sonarqubecloud bot commented May 2, 2024

Quality Gate passed

werli commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

Stephan202 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2024

Stephan202 commented Nov 3, 2024

Stephan202 commented Nov 4, 2024

werli left a comment • edited Loading

Choose a reason for hiding this comment

Stephan202 commented Nov 4, 2024

Stephan202 commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

sonarqubecloud bot commented Nov 11, 2024

Quality Gate passed

werli commented Nov 11, 2024

Introduce assorted Reactor `StepVerifier` Refaster rules #1132

Introduce assorted Reactor `StepVerifier` Refaster rules #1132

werli commented Apr 11, 2024 •

edited

Loading

rickie left a comment •

edited

Loading

werli left a comment •

edited

Loading